Verify prebuild metadata and JS addon precedence - #455
Conversation
kraenhansen
left a comment
There was a problem hiding this comment.
Compared this against the other open attempt at #424 (#426) and against #450 (closed, but its rationale is directly relevant here). Verified locally by checking out both branches, building, and running the affected test files.
The precedence fix only covers one of the two call sites that need it. resolvesToNonAddon (packages/host/src/node/babel-plugin/plugin.ts:70-76) guards the plain require(id) branch at lines 111-115, but the require('bindings')(name) branch just above it (lines 99-109) calls findNodeAddonForBindings, which internally calls the unpatched isNodeApiModule in path-utils.ts — so a .js/.json file that shadows a same-named .node addon still gets misdetected as the addon when reached via bindings(). This is exactly why #450 (built on top of #426) moved the fix into the shared isNodeApiModule utility instead of guarding only the plugin's require() handling — same root cause, both callers fixed. Worth confirming with a bindings() fixture analogous to the one added to plugin.test.ts.
Fixing this via createRequire(...).resolve() at transform time is also the approach #450 deliberately avoided, for a reason worth weighing here too: this runs Node's own CommonJS resolution algorithm to predict what Metro will resolve at bundle time, but Metro has its own resolver (platform extensions, Haste, etc.) that can diverge from Node's. It's also fail-open — any exception from .resolve() (e.g. a platform-specific .ios.js that Node's resolver doesn't know how to find) is swallowed and falls back to the old (buggy) behavior. A direct sibling-file check in isNodeApiModule, as #426/#450 do, doesn't depend on that assumption holding.
Base branch: this targets main, while #424's other three attempts (#426, #448, #449, #450) all targeted next, which currently has real work main lacks. git merge-tree shows this produces actual conflicts against next (root package.json's build/copy-and-build/test scripts, and the DIRS-vs-EXAMPLES_DIR loop in verify-prebuilds.mts) — worth rebasing onto/retargeting next before this can land there.
One genuine improvement worth keeping regardless of outcome here: verify-prebuilds.test.mts unit-tests verifyFrameworkInfoPlist directly (3/3 passing locally) and wires it into the fast root pnpm test gate via test:verify, without requiring a full native build — #426 only exercises its Info.plist check through the full node-addon-examples build+verify flow. That's a nice piece to carry over into whichever PR ends up merged.
Generated by Claude Code
|
Confirmed the review against the current
Keeping #455 active would duplicate the approved contribution, so I am closing it in favor of #426. The independent |
Summary
.nodefixture to the JS require precedence testInfo.plistand verifyCFBundleExecutableplus the expected escapedCFBundleIdentifierThe completed Babel fixture exposed a real bug: current code transformed
require("./my-addon")intorequireNodeAddon(...)even whenmy-addon.jsexisted. This PR fixes that precedence rather than weakening the new test.Verification
requireNodeAddonpnpm run buildpnpm test: 88 tests passed (62 host, 20 gyp-to-cmake, 3 cmake-rn, 3 plist verifier)pnpm run prettier:checkpnpm run depcheckpnpm run publintFull root lint additionally requires generated native typings from the Rust/clang-format bootstrap; those toolchains are not installed locally. The changed files are lint-clean, and CI runs the documented bootstrap before its root lint job.
Fixes #424